Introduce `OptimizerRule::rewrite` to rewrite in place, rewrite `ExprSimplifier` (20% faster planning) #9954

alamb · 2024-04-04T20:16:47Z

Note: A substantial number of the changes in this PR are removing & in tests. The actual logic change is quite small

Which issue does this PR close?

Part of #9637

Rationale for this change

The less copying in the planner, the less time it takes to plan a query. Profiling from thesql_planner benchmark in #9637 shows almost 25% of the time is spent in ExprSimplifier, so let's make that faster.

The current API of OptimizerRule s requires a copy of the LogicalPlan to be made. Thus we need a new API that allows for in place rewrites.

What changes are included in this PR?

Add new OptimizerRule::rewrite API for in place rewrites
Update Optimizer to use the new API
Update ExprSimplifier to implement rewrite rather than try_optimize

Open questions:

should we deprecate the old OptimizerRule API (I think so -- maybe we can do this as a follow on PR)

Are these changes tested?

Yes by existing tests

Performance benchmark: 22% faster TPC-H planning, 26% faster TPC-DS planning

Details

++ critcmp main simplify_exprs_no_copy
group                                         main                                   simplify_exprs_no_copy
-----                                         ----                                   ----------------------
logical_aggregate_with_join                   1.00  1189.1±11.74µs        ? ?/sec    1.00  1186.6±10.71µs        ? ?/sec
logical_plan_tpcds_all                        1.00    155.7±0.85ms        ? ?/sec    1.00    155.6±0.98ms        ? ?/sec
logical_plan_tpch_all                         1.00     16.7±0.18ms        ? ?/sec    1.00     16.7±0.15ms        ? ?/sec
logical_select_all_from_1000                  1.06     19.3±0.11ms        ? ?/sec    1.00     18.2±0.56ms        ? ?/sec
logical_select_one_from_700                   1.00   782.8±19.70µs        ? ?/sec    1.00    780.6±8.10µs        ? ?/sec
logical_trivial_join_high_numbered_columns    1.00   732.4±18.61µs        ? ?/sec    1.00    732.4±9.34µs        ? ?/sec
logical_trivial_join_low_numbered_columns     1.00    714.8±6.80µs        ? ?/sec    1.00    716.0±6.69µs        ? ?/sec
physical_plan_tpcds_all                       1.26   1847.2±3.05ms        ? ?/sec    1.00   1470.4±3.99ms        ? ?/sec
physical_plan_tpch_all                        1.22    119.8±0.74ms        ? ?/sec    1.00     98.2±0.66ms        ? ?/sec
physical_plan_tpch_q1                         1.37      7.3±0.07ms        ? ?/sec    1.00      5.4±0.02ms        ? ?/sec
physical_plan_tpch_q10                        1.20      5.5±0.03ms        ? ?/sec    1.00      4.6±0.03ms        ? ?/sec
physical_plan_tpch_q11                        1.20      4.8±0.03ms        ? ?/sec    1.00      4.0±0.02ms        ? ?/sec
physical_plan_tpch_q12                        1.19      3.9±0.03ms        ? ?/sec    1.00      3.3±0.02ms        ? ?/sec
physical_plan_tpch_q13                        1.18      2.6±0.01ms        ? ?/sec    1.00      2.2±0.01ms        ? ?/sec
physical_plan_tpch_q14                        1.18      3.4±0.02ms        ? ?/sec    1.00      2.9±0.01ms        ? ?/sec
physical_plan_tpch_q16                        1.23      4.9±0.03ms        ? ?/sec    1.00      4.0±0.02ms        ? ?/sec
physical_plan_tpch_q17                        1.24      4.6±0.03ms        ? ?/sec    1.00      3.8±0.02ms        ? ?/sec
physical_plan_tpch_q18                        1.22      5.0±0.03ms        ? ?/sec    1.00      4.1±0.02ms        ? ?/sec
physical_plan_tpch_q19                        1.21      9.4±0.15ms        ? ?/sec    1.00      7.8±0.15ms        ? ?/sec
physical_plan_tpch_q2                         1.24     10.5±0.07ms        ? ?/sec    1.00      8.5±0.06ms        ? ?/sec
physical_plan_tpch_q20                        1.26      6.1±0.04ms        ? ?/sec    1.00      4.8±0.03ms        ? ?/sec
physical_plan_tpch_q21                        1.24      8.3±0.05ms        ? ?/sec    1.00      6.7±0.05ms        ? ?/sec
physical_plan_tpch_q22                        1.23      4.4±0.03ms        ? ?/sec    1.00      3.6±0.02ms        ? ?/sec
physical_plan_tpch_q3                         1.18      3.9±0.03ms        ? ?/sec    1.00      3.3±0.02ms        ? ?/sec
physical_plan_tpch_q4                         1.20      2.9±0.02ms        ? ?/sec    1.00      2.4±0.03ms        ? ?/sec
physical_plan_tpch_q5                         1.16      5.6±0.04ms        ? ?/sec    1.00      4.8±0.03ms        ? ?/sec
physical_plan_tpch_q6                         1.15  1996.9±10.25µs        ? ?/sec    1.00  1739.6±86.99µs        ? ?/sec
physical_plan_tpch_q7                         1.22      7.5±0.11ms        ? ?/sec    1.00      6.2±0.07ms        ? ?/sec
physical_plan_tpch_q8                         1.20      9.6±0.05ms        ? ?/sec    1.00      8.0±0.07ms        ? ?/sec
physical_plan_tpch_q9                         1.21      7.2±0.04ms        ? ?/sec    1.00      6.0±0.03ms        ? ?/sec
physical_select_all_from_1000                 1.47    128.4±0.49ms        ? ?/sec    1.00     87.1±0.31ms        ? ?/sec
physical_select_one_from_700                  1.10      4.0±0.03ms        ? ?/sec    1.00      3.7±0.02ms        ? ?/sec

Are there any user-facing changes?

Faster planning 🚀

…lifyExprs` to avoid copies

alamb · 2024-04-10T13:33:38Z

datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs


-        plan.with_new_exprs(exprs, new_inputs)


The whole point of this PR is to remove the calls to expressions and new_with_exprs (which requires cloning all the expressions)

alamb · 2024-04-10T13:33:40Z

datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs

@@ -109,18 +127,22 @@ impl SimplifyExpressions {
            simplifier
        };

-        let exprs = plan
-            .expressions()


Calling expressions() clones all the expressions

alamb · 2024-04-10T19:26:35Z

@jackwener and @jayzhan211 here the next PR in the "don't copy so much in the optimizer"

jayzhan211 · 2024-04-10T23:55:27Z

datafusion/optimizer/src/optimizer.rs

    ///
-    /// If a rule use default None, it should traverse recursively plan inside itself
+    /// If returns `None`, the default, the rule must handle recursion itself
    fn apply_order(&self) -> Option<ApplyOrder> {
        None


I think most of the cases are BottomUp by default?

I think most of the cases are BottomUp by default?

we can't assume it.

I agree most of the cases probably should be bottom up (but not all). I'll keep an eye open for this as I work through the other optimizer rules

jayzhan211

👍

alamb · 2024-04-11T10:22:28Z

Thanks @jackwener and @jayzhan211 -- I assumed it would take some time to review these PRs but you are so efficient!

I plan to spend a little time planning today to write down the remaining tasks (aka rewriting each OptimizerRule to use the new APIs) and then start cranking on them. CommonSubExprEliminate will be a tricky one but another major win I think

…lifyExprs` to avoid copies (apache#9954)

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Apr 4, 2024

alamb force-pushed the alamb/simplify_exprs_no_copy branch from 4f99bfc to 6be96c2 Compare April 5, 2024 10:06

alamb changed the title ~~Introduce OptimizerRule::try_optimize_owned to rewrite in place~~ Introduce OptimizerRule::try_optimize_owned to rewrite in place, rewrite ExprSimplifier Apr 5, 2024

alamb changed the title ~~Introduce OptimizerRule::try_optimize_owned to rewrite in place, rewrite ExprSimplifier~~ Introduce OptimizerRule::try_optimize_owned to rewrite in place, rewrite ExprSimplifier Apr 5, 2024

This was referenced Apr 5, 2024

[EPIC] Stop copying LogicalPlan during OptimizerPasses #9637

Closed

Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit #9913

Merged

alamb force-pushed the alamb/simplify_exprs_no_copy branch 3 times, most recently from 168f63f to dacbfa6 Compare April 7, 2024 18:00

alamb changed the title ~~Introduce OptimizerRule::try_optimize_owned to rewrite in place, rewrite ExprSimplifier~~ Introduce OptimizerRule::rewrite to rewrite in place, rewrite ExprSimplifier Apr 10, 2024

alamb force-pushed the alamb/simplify_exprs_no_copy branch from dacbfa6 to d95eca4 Compare April 10, 2024 11:01

github-actions bot removed the logical-expr Logical plan and expressions label Apr 10, 2024

alamb force-pushed the alamb/simplify_exprs_no_copy branch from d95eca4 to 6d31211 Compare April 10, 2024 11:20

alamb changed the title ~~Introduce OptimizerRule::rewrite to rewrite in place, rewrite ExprSimplifier~~ Introduce OptimizerRule::rewrite to rewrite in place, rewrite ExprSimplifier (20% faster planning) Apr 10, 2024

Introduce OptimizerRule::rewrite to rewrite in place, Rewrite `Simp…

e31a358

…lifyExprs` to avoid copies

alamb force-pushed the alamb/simplify_exprs_no_copy branch from c9ff49b to e31a358 Compare April 10, 2024 13:35

github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Apr 10, 2024

alamb commented Apr 10, 2024

View reviewed changes

alamb marked this pull request as ready for review April 10, 2024 13:50

jayzhan211 reviewed Apr 10, 2024

View reviewed changes

jayzhan211 approved these changes Apr 10, 2024

View reviewed changes

jackwener approved these changes Apr 11, 2024

View reviewed changes

jackwener merged commit 58e0b59 into apache:main Apr 11, 2024
24 checks passed

alamb deleted the alamb/simplify_exprs_no_copy branch April 11, 2024 10:19

appletreeisyellow pushed a commit to influxdata/arrow-datafusion that referenced this pull request Apr 24, 2024

Introduce OptimizerRule::rewrite to rewrite in place, Rewrite `Simp…

37c22e4

…lifyExprs` to avoid copies (apache#9954)

backkem mentioned this pull request Aug 23, 2024

Migrate SQLFederationOptimizerRule to OptimizerRule::rewrite datafusion-contrib/datafusion-federation#46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `OptimizerRule::rewrite` to rewrite in place, rewrite `ExprSimplifier` (20% faster planning) #9954

Introduce `OptimizerRule::rewrite` to rewrite in place, rewrite `ExprSimplifier` (20% faster planning) #9954

alamb commented Apr 4, 2024 •

edited

Loading

alamb Apr 10, 2024

alamb Apr 10, 2024 •

edited

Loading

alamb commented Apr 10, 2024

jayzhan211 Apr 10, 2024

jackwener Apr 11, 2024

alamb Apr 11, 2024

jayzhan211 left a comment

alamb commented Apr 11, 2024

Introduce OptimizerRule::rewrite to rewrite in place, rewrite ExprSimplifier (20% faster planning) #9954

Introduce OptimizerRule::rewrite to rewrite in place, rewrite ExprSimplifier (20% faster planning) #9954

Conversation

alamb commented Apr 4, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb Apr 10, 2024

Choose a reason for hiding this comment

alamb Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

alamb commented Apr 10, 2024

jayzhan211 Apr 10, 2024

Choose a reason for hiding this comment

jackwener Apr 11, 2024

Choose a reason for hiding this comment

alamb Apr 11, 2024

Choose a reason for hiding this comment

jayzhan211 left a comment

Choose a reason for hiding this comment

alamb commented Apr 11, 2024

Introduce `OptimizerRule::rewrite` to rewrite in place, rewrite `ExprSimplifier` (20% faster planning) #9954

Introduce `OptimizerRule::rewrite` to rewrite in place, rewrite `ExprSimplifier` (20% faster planning) #9954

alamb commented Apr 4, 2024 •

edited

Loading

alamb Apr 10, 2024 •

edited

Loading